Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

smp: disable mlock on overcommit #2575

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tomershafir
Copy link
Contributor

Memory locking keeps pages in physical memory, and decreases the amount of pages available for swapping on an overcommit setup. Thus, it can decrease fairness, increase memory pressure, and cause OOMs. So, it is less suitable for a cooperative overcommit setup.

This change disables memory locking on overcommit (overprovisioned - I hope I can rename that config). Also, it reuses the mlock variable. I assume that this change shouldn't break reasonable configs.

@tomershafir tomershafir marked this pull request as ready for review December 11, 2024 19:44
@@ -3814,7 +3814,7 @@ smp_options::smp_options(program_options::option_group* parent_group)
, memory(*this, "memory", std::nullopt, "memory to use, in bytes (ex: 4G) (default: all)")
, reserve_memory(*this, "reserve-memory", {}, "memory reserved to OS (if --memory not specified)")
, hugepages(*this, "hugepages", {}, "path to accessible hugetlbfs mount (typically /dev/hugepages/something)")
, lock_memory(*this, "lock-memory", {}, "lock all memory (prevents swapping)")
, lock_memory(*this, "lock-memory", {}, "lock all memory (prevents swapping). Disable for overprovisioned environments")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The help text for --overprovisioned should be updated too

@xemul xemul requested a review from avikivity December 24, 2024 08:30
Memory locking keeps pages in physical memory, and decreases the amount of pages available for swapping on an overcommit setup. Thus, it can decrease fairness, increase memory pressure, and cause OOMs. So, it is less suitable for a cooperative overcommit setup.

This change disables memory locking on overcommit (overprovisioned - I hope I can rename that config). Also, it reuses the mlock variable. I assume that this change shouldn't break reasonable configs.
@tomershafir tomershafir force-pushed the smp-mlock-disable-on-overcommit branch from cc1b6bb to 3e4d3eb Compare December 24, 2024 09:35
@tomershafir tomershafir requested a review from xemul December 24, 2024 09:36
@avikivity
Copy link
Member

Wasn't mlock default off for all environments?

@@ -4332,7 +4332,7 @@ void smp::configure(const smp_options& smp_opts, const reactor_options& reactor_
hugepages_path = smp_opts.hugepages.get_value();
}
auto mlock = false;
if (smp_opts.lock_memory) {
if (smp_opts.lock_memory && !reactor_opts.overprovisioned) {
mlock = smp_opts.lock_memory.get_value();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's default off, so why this extra work?

If someone specified both --overprovisioned (which really should have been named --overcommitted) and --lock-memory 1, we can assume they know what they're doing. Maybe only the CPU is overcommitted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants